Skip to content

Conversation

@xurui-c
Copy link
Member

@xurui-c xurui-c commented Jul 10, 2025

Screenshot 2025-07-09 at 7 12 02 PM

@codecov
Copy link

codecov bot commented Jul 10, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1 1 0 0
View the top 1 failed test(s) by shortest run time
::tests.query.allocation_policies.test_allocation_policy_base
Stack Traces | 0s run time
ImportError while importing test module '.../query/allocation_policies/test_allocation_policy_base.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
.../local/lib/python3.11.../site-packages/_pytest/python.py:493: in importtestmodule
    mod = import_path(
.../local/lib/python3.11.../site-packages/_pytest/pathlib.py:582: in import_path
    importlib.import_module(module_name)
.../local/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1204: in _gcd_import
    ???
<frozen importlib._bootstrap>:1176: in _find_and_load
    ???
<frozen importlib._bootstrap>:1147: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:690: in _load_unlocked
    ???
.../local/lib/python3.11.../_pytest/assertion/rewrite.py:184: in exec_module
    exec(co, module.__dict__)
.../query/allocation_policies/test_allocation_policy_base.py:8: in <module>
    from snuba.query.allocation_policies import (
E   ImportError: cannot import name 'InvalidPolicyConfig' from 'snuba.query.allocation_policies' (.../query/allocation_policies/__init__.py)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@xurui-c xurui-c force-pushed the rachel/ui branch 2 times, most recently from b84844a to ed9b838 Compare July 10, 2025 02:40
@xurui-c xurui-c force-pushed the rachel/ui branch 3 times, most recently from f5fbbd4 to ea79b3e Compare July 31, 2025 18:59
@@ -0,0 +1,218 @@
from dataclasses import dataclass, field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong folder, find a better place for it or make it



@dataclass()
class Configuration:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vibe: this is a pretty generic name for something, may be helpful to make it more specific

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the very least, you should explain what this is

}


class ConfigurableComponent:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usage example in the docstring

Comment on lines +97 to +109
def get_config_value(
self, config_key: str, params: dict[str, Any] = {}, validate: bool = True
) -> Any:
pass

def set_config_value(
self,
config_key: str,
value: Any,
params: dict[str, Any] = {},
user: str | None = None,
) -> None:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should not have to implemented by the user, should be plug and play

key = key.replace(delimiter_char, escape_sequence)
return key

def _build_runtime_config_key(self, config: str, params: dict[str, Any]) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably include the component name

Comment on lines +67 to +69
# what is this configurable component?
# allocation policy? routing strategy? strategy selector?
return self.__class__.__name__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be what runtime_config_prefix is

Comment on lines +177 to +184
self._configurations = [
RoutingStrategyConfig(
name="max_load",
description="The maximum load we allow the Clickhouse cluster to reach",
value_type=int,
default=100,
),
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return this from get_configurations

Comment on lines +692 to +694
def component_namespace(self) -> str:
return "allocation_policy"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to ignore all existing allocation policy configurations. Make sure they are preserved

Comment on lines +4 to +9
interface ConfigurableComponent {
type: string;
name: string;
configs: Configuration[];
optional_config_definitions: OptionalConfigurationDefinition[];
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're making a more generic thing, it should not be housed under capacity management

Comment on lines +58 to +59
entity={{ type: "storage", name: selectedStorage }}
configurable_component={policy}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be hardcoded, the configurable component should return its type

@xurui-c xurui-c changed the title feat(cbrs): UI cbrs UI test pr (meant to be used to play around, won't merge) Aug 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants